Fix build of v3.6 (issues #9186 and #9188) #9189

misch7 · 2024-05-27T18:47:02Z

Description

Fix two build issues related to custom configs:

Fix Build library failed for server mode only: missing-prototypes for mbedtls_ssl_conf_has_static_psk #9186
Fix Build fails with unset MBEDTLS_DHM_C but MBEDTLS_USE_PSA_CRYPTO set #9188

Also fix additional build issues with sample programs and test suites caused by introducing -Wmissing-prototypes

PR checklist

changelog provided
development PR provided
framework PR provided Generate static test functions & wrappers mbedtls-framework#23
3.6 PR provided [Backport 3.6] Fix v3.6 issues 9186 and 9188 bp #9451
2.28 PR not required because: neither issue is present in 2.28 (Build library failed for server mode only: missing-prototypes for mbedtls_ssl_conf_has_static_psk #9186: no build failure in the given configuration and compilers; Build fails with unset MBEDTLS_DHM_C but MBEDTLS_USE_PSA_CRYPTO set #9188: moot since PSA did not support DHM yet)
tests provided. Some more CI coverage required, added -Wmissing-prototypes to all.sh

gilles-peskine-arm

Thanks for raising and fixing these issues!

For bug fixes, we want to have a changelog entry and, when feasible, a non-regression test. Here the non-regression test is to up the warning level in a CI build, and add a configuration but I'm not sure what configuration yet, I'll think about it.

library/ssl_misc.h

gilles-peskine-arm · 2024-05-27T21:06:03Z

library/ssl_tls12_server.c

           The size of the array is thus greater than 256 bytes which is greater than any
           possible value of ecpoint_len (type uint8_t) and the check below can be skipped.*/


When FFDH is disabled but RSA is enabled, sizeof(handshake->xxdh_psa_peerkey) is also greater than 256 bytes (assuming a sensible maximum RSA key size — if I got the math right, setting PSA_VENDOR_RSA_MAX_KEY_BITS to 1912 or less would result in ≤255 bytes for sizeof(handshake->xxdh_psa_peerkey) which is PSA_EXPORT_PUBLIC_KEY_MAX_SIZE).

This doesn't need to be the case, since RSA is irrelevant to what's stored in xxdh_psa_peerkey, but the size of the array is declared as PSA_EXPORT_PUBLIC_KEY_MAX_SIZE and not as max(max FFDH public key size, max ECC public key size).

Skipping this check when FFDH is disabled was done in 24e50d3 to silence this compiler warning. We could broaden the skip to whenever the buffer is larger than UINT8_MAX. But I don't like the way this is done: I'd prefer to let compilers figure out if the comparison is always true and optimize the branch out. Evidently Clang, at least, figures it out.

I'd like a second opinion from another Mbed TLS maintainer on the preferred fix here.

Thanks for the explanation. It's interesting to learn a bit about the internals of the library. Without knowing all the implications, I'd in general prefer as well to let compilers figure out the comparison. This already came in handy here with Clang forcing me to look at potential mistakes without letting them slip through.

library/ssl_tls12_server.c

library/ssl_misc.h

misch7 · 2024-05-28T00:53:16Z

Thanks for raising and fixing these issues!

For bug fixes, we want to have a changelog entry and, when feasible, a non-regression test. Here the non-regression test is to up the warning level in a CI build, and add a configuration but I'm not sure what configuration yet, I'll think about it.

Thanks for the fast review and detailed explanation!

I'd also advocate for adding a configuration even if it's notably hard to cover all scenarios.

gilles-peskine-arm

Thanks for the update! Looks good to me, except that adding -Wmissing-prototypes causes another warning, looking like a missing #include. Please fix that one as well since the CI is now failing.

We're still missing a non-regression test for the no-DHM configuration, but that's a more general issue in our testing for which we need to do a bit of design, so please don't bother with that one, we're going to discuss and resolve it internally.

tests/scripts/all.sh

gilles-peskine-arm

Wow, I didn't expect this many changes! But thanks for following through. Note, if it wasn't for #9218, I'd say it isn't worth it to fiddle with tests/*. But if there's a concrete problem with building the unit tests on NuttX, then it's worth making it possible.

I'm afraid I'm going to request that you please force-push a new history that does not add static to test case functions, i.e. the lines after BEGIN_CASE in *.function. Requiring static there is just noise. Also, please avoid rewrapping lines in the same commit: that makes the commits hard to review. For reviewing, I'd like the additions of static to disappear in

git log -p -w dev..HEAD | perl -0777 -pe 's/-(.*\n)\+static \1//gm; s/^@.*\n(?: .*\n)*(?=@|diff|\Z)//gm; s/^diff.*\nindex.*\n--- .*\n\+\+\+ .*\n(?=\Z|diff |commit )//gm' | colordiff | less -R

so that we can focus on what else is happening.

gilles-peskine-arm · 2024-06-02T19:56:41Z

tests/scripts/generate_test_code.py

@@ -200,7 +200,7 @@
 CONDITION_REGEX = r'({})(?:\s*({})\s*({}))?$'.format(C_IDENTIFIER_REGEX,
                                                     CONDITION_OPERATOR_REGEX,
                                                     CONDITION_VALUE_REGEX)
-TEST_FUNCTION_VALIDATION_REGEX = r'\s*void\s+(?P<func_name>\w+)\s*\('
+TEST_FUNCTION_VALIDATION_REGEX = r'\s*static\s*void\s+(?P<func_name>\w+)\s*\('


Please don't require static in the input. We should keep writing

void foo(…)

in the .function files, and it's up to the generator script to munge the function name, and now add static.

Done in d2e3dc9 and Mbed-TLS/mbedtls-framework#23

tests/suites/test_suite_psa_crypto.function

gilles-peskine-arm · 2024-06-02T20:13:35Z

tests/scripts/all.sh

@@ -4385,14 +4385,14 @@ component_build_no_ssl_srv () {
    msg "build: full config except SSL server, make, gcc" # ~ 30s
    scripts/config.py full
    scripts/config.py unset MBEDTLS_SSL_SRV_C
-    make CC=gcc CFLAGS='-Werror -Wall -Wextra -O1'
+    make CC=gcc CFLAGS='-Werror -Wall -Wextra -O1 -Wmissing-prototypes'
 }


We're currently passing -Wmissing-prototypes when building with CMake, but only inside library. Please move the corresponding clauses from library/CMakeLists.txt to the toplevel cmakelists.

Just to be sure, do you only refer to the -Wmissing-prototypes clauses within these lines:

From GCC:

mbedtls/library/CMakeLists.txt

Line 200 in 98ffc8e

set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wmissing-declarations -Wmissing-prototypes")

From Clang:

mbedtls/library/CMakeLists.txt

Line 204 in 98ffc8e

set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wmissing-declarations -Wmissing-prototypes -Wdocumentation -Wno-documentation-deprecated-sync -Wunreachable-code")

...to be moved to these lines:

To GCC:

mbedtls/CMakeLists.txt

Line 199 in 98ffc8e

set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -Wextra -Wwrite-strings")

To Clang:

mbedtls/CMakeLists.txt

Line 233 in 98ffc8e

set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -Wextra -Wwrite-strings -Wpointer-arith -Wimplicit-fallthrough -Wshadow -Wvla -Wformat=2 -Wno-format-nonliteral")

...or all / other specific clauses from the original lines?

And are the target lines correct?

Just moving -Wmissing-prototypes, not the other -W. I think the target lines are correct, or maybe the GCC line needs to be in a version-based conditional, I don't know when -Wmissing-prototypes was introduces.

Ok, thanks.

Meanwhile I've dug a bit and the oldest reference I could find in GCC's repo is from 1999:
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=fe50c0eb3bcd01019e3b8e610b3906510a5d4b8e

That's from version 2.96 19991014:
https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/version.c;h=2c4f8fbc6ca55f810c8c8e306cee5895e9ae3eae;hb=fe50c0eb3bcd01019e3b8e610b3906510a5d4b8e

So I guess given the age and that it worked in library all the time, we can safely assume that it will work.

Done in 97d20d5

I've just seen that the previous CI job failed with:

tests/suites/test_suite_ctr_drbg.function:99:14: error: unused function 'thread_random_function' [-Werror,-Wunused-function]

Guess I have to figure out a way to build exactly the same locally.

So I've managed to reproduce the CI failure by calling what I scrapped from the logs:

tests/scripts/all.sh --seed 4 --keep-going test_cmake_out_of_source

Before I ran into the same issue as the CI did, a few more errors were raised on each run:

tests/src/drivers/platform_builtin_keys.c:59:14: error: no previous prototype for function 'mbedtls_psa_platform_get_builtin_key'

And after making the former function static, two unused errors occurred:

tests/src/drivers/platform_builtin_keys.c:59:21: error: unused function 'mbedtls_psa_platform_get_builtin_key' tests/src/drivers/platform_builtin_keys.c:26:52: error: unused variable 'builtin_keys'

When temporarily commenting those out, I finally ran into the same error as the CI did:

tests/suites/test_suite_ctr_drbg.function:99:14: error: unused function 'thread_random_function'

Which is actually not true, because grep revealed that thread_random_function is used within the same file at line 402. It's apparently hard to figure it out for the compiler because it is being used in the TEST_EQUAL macro:

mbedtls/tests/suites/test_suite_ctr_drbg.function

Lines 399 to 404 in 7777cb7

for (size_t i = 0; i < thread_count; i++) {

TEST_EQUAL(

mbedtls_test_thread_create(&threads[i],

thread_random_function, (void *) &ctx),

0);

}

My suggestion would be to make thread_random_function a regular function again and add its prototype directly in the line 99 above the definition:

mbedtls/tests/suites/test_suite_ctr_drbg.function

Line 99 in 7777cb7

static void *thread_random_function(void *ctx)

The build and all tests successfully passed, once I did this locally and deleted the unused file tests/src/drivers/platform_builtin_keys.c as well :)

Shall I proceed as proposed?

I'm not quite sure because of possible implications of mbedtls_psa_platform_get_builtin_key being referenced here when MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS is enabled (which is disabled by default):

mbedtls/tests/src/psa_test_wrappers.c

Lines 29 to 39 in 7777cb7

/* Wrapper for mbedtls_psa_platform_get_builtin_key */

#if defined(MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS)

psa_status_t mbedtls_test_wrap_mbedtls_psa_platform_get_builtin_key(

mbedtls_svc_key_id_t arg0_key_id,

psa_key_lifetime_t *arg1_lifetime,

psa_drv_slot_number_t *arg2_slot_number)

{

psa_status_t status = (mbedtls_psa_platform_get_builtin_key)(arg0_key_id, arg1_lifetime, arg2_slot_number);

return status;

}

#endif /* defined(MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS) */

mbedtls/include/mbedtls/mbedtls_config.h

Lines 1325 to 1341 in 7777cb7

/** \def MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS

*

* Enable support for platform built-in keys. If you enable this feature,

* you must implement the function mbedtls_psa_platform_get_builtin_key().

* See the documentation of that function for more information.

*

* Built-in keys are typically derived from a hardware unique key or

* stored in a secure element.

*

* Requires: MBEDTLS_PSA_CRYPTO_C.

*

* \warning This interface is experimental and may change or be removed

* without notice.

*/

//#define MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS

thread_random_function is defined unconditionally but only used under certain conditions: TEST_CASE depends_on:MBEDTLS_THREADING_PTHREAD:!MBEDTLS_CTR_DRBG_USE_128_BIT_KEY:!MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH means

#if defined(MBEDTLS_THREADING_PTHREAD) && !defined(MBEDTLS_CTR_DRBG_USE_128_BIT_KEY) && !defined(MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH)

(I'm not sure why it's 128-bit-AES only.) Either the function needs to remain non-static and to have a prototype, or its definition needs to have a matching guard.

As for mbedtls_psa_platform_get_builtin_key, it's used when building tests in configurations with MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS enabled, such as the configuration obtained with scripts/config.py full.

Thanks for the clarification, done in d2e3dc9

tests/scripts/all.sh

gilles-peskine-arm · 2024-06-02T20:22:54Z

tests/src/asn1_helpers.c

@@ -15,6 +15,8 @@

 #include <mbedtls/asn1.h>

+#include <test/asn1_helpers.h>


Also, for a change of this magnitude, I think we need a 2.28 backport as well, just for the parts that backport cleanly. I don't particularly care to pass -Wmissing-prototypes in 2.28, but I want backporting changes to tests to remain easy when it's currently a simple cherry-pick. For parts that are already different, it doesn't matter.

Who shall do the backporting?

Ideally you, after the main pull request is approved. But if we've exhausted your bandwidth someone else will deal with it, it just might take more time.

Ok, I'm happy to do it.

gilles-peskine-arm · 2024-06-05T13:12:56Z

I'm afraid there's a merge conflict now, so I must ask you to please rebase again.

…rors in mbedtls/tests/suites Required for: Mbed-TLS/mbedtls#9189 Signed-off-by: Michael Schuster <michael@schuster.ms>

gilles-peskine-arm · 2024-08-09T15:43:42Z

The CI is very unhappy. I don't know what's going on. A mismatch between the framework version and the main repo version? So maybe Mbed-TLS/mbedtls-framework#23 needs to be rebased on top of the latest main?

…rors in mbedtls/tests/suites Required for: Mbed-TLS/mbedtls#9189 Signed-off-by: Michael Schuster <michael@schuster.ms>

minosgalanakis · 2024-08-09T16:15:41Z

The CI is very unhappy. I don't know what's going on. A mismatch between the framework version and the main repo version? So maybe Mbed-TLS/mbedtls-framework#23 needs to be rebased on top of the latest main?

This is bizzare indeed. In the 3.6 which is pointing at the same framework branch it passes, so if I rebase I will have to re-kick that CI job too.

misch7 · 2024-08-09T17:16:57Z

@misch7 Since I already did the conflict resolution for the rebase, I cherry picked the commit a368c8a on this branch and rebased.

I hope that is ok. If CI is clear, I will be reviewing it at high priority.

@minosgalanakis Ok, thanks!

misch7 · 2024-08-09T17:18:05Z

The CI is very unhappy. I don't know what's going on. A mismatch between the framework version and the main repo version? So maybe Mbed-TLS/mbedtls-framework#23 needs to be rebased on top of the latest main?

@gilles-peskine-arm Done, hope this helps.

See Mbed-TLS/mbedtls-framework#23 Signed-off-by: Michael Schuster <michael@schuster.ms>

misch7 · 2024-08-09T18:52:04Z

While testing the recent rebase locally, I apparently ran into the same issue as the CI did:

$ make clean
Traceback (most recent call last):
  File "mbedtls/tests/../framework/scripts/generate_config_tests.py", line 179, in <module>
    test_data_generation.main(sys.argv[1:], __doc__, ConfigTestGenerator)
  File "mbedtls/framework/scripts/mbedtls_framework/test_data_generation.py", line 204, in main
    generator = generator_class(options)
                ^^^^^^^^^^^^^^^^^^^^^^^^
  File "mbedtls/tests/../framework/scripts/generate_config_tests.py", line 162, in __init__
    self.mbedtls_config = config.ConfigFile()
                          ^^^^^^^^^^^^^^^^^^^
TypeError: ConfigFile.__init__() missing 2 required positional arguments: 'default_path' and 'name'

The problem went away when built with the rebased framework commit: Mbed-TLS/mbedtls-framework@6a1dc7d

Hence I pushed b77c419 - fingers crossed for the CI.

gilles-peskine-arm

LGTM after submodule update

eleuzi01

LGTM, thanks!

gilles-peskine-arm requested changes May 27, 2024

View reviewed changes

gilles-peskine-arm added needs-work component-tls needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon labels May 27, 2024

misch7 requested a review from gilles-peskine-arm May 28, 2024 00:54

gilles-peskine-arm requested changes May 29, 2024

View reviewed changes

tests/scripts/all.sh Outdated Show resolved Hide resolved

gilles-peskine-arm added needs-ci Needs to pass CI tests needs-work and removed needs-work needs-ci Needs to pass CI tests labels May 29, 2024

misch7 requested a review from gilles-peskine-arm June 2, 2024 00:18

misch7 mentioned this pull request Jun 2, 2024

Make local functions and objects static #9218

Closed

4 tasks

gilles-peskine-arm requested changes Jun 2, 2024

View reviewed changes

gilles-peskine-arm added size-s Estimated task size: small (~2d) and removed needs-reviewer This PR needs someone to pick it up for review labels Jun 2, 2024

gilles-peskine-arm reviewed Jun 2, 2024

View reviewed changes

misch7 force-pushed the fix-v3.6-issues-9186-and-9188 branch from ff96171 to 7777cb7 Compare June 4, 2024 00:50

misch7 requested a review from gilles-peskine-arm June 4, 2024 01:36

gilles-peskine-arm mentioned this pull request Jun 4, 2024

Fix uint32_t printed as unsigned int #9225

Merged

4 tasks

misch7 mentioned this pull request Jun 5, 2024

Generate static test functions & wrappers Mbed-TLS/mbedtls-framework#23

Merged

misch7 force-pushed the fix-v3.6-issues-9186-and-9188 branch from 815f555 to f49ee2a Compare June 5, 2024 19:06

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jun 5, 2024

gilles-peskine-arm requested a review from eleuzi01 August 9, 2024 14:42

eleuzi01 previously approved these changes Aug 9, 2024

View reviewed changes

eleuzi01 removed the needs-review Every commit must be reviewed by at least two team members, label Aug 9, 2024

gilles-peskine-arm added needs-work and removed approved Design and code approved - may be waiting for CI or backports labels Aug 9, 2024

Update the submodule to the head of PR in the framework repository

b77c419

See Mbed-TLS/mbedtls-framework#23 Signed-off-by: Michael Schuster <michael@schuster.ms>

misch7 dismissed stale reviews from eleuzi01 and gilles-peskine-arm via b77c419 August 9, 2024 18:40

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work needs-ci Needs to pass CI tests labels Aug 10, 2024

gilles-peskine-arm approved these changes Aug 10, 2024

View reviewed changes

eleuzi01 approved these changes Aug 12, 2024

View reviewed changes

eleuzi01 added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Aug 12, 2024

gilles-peskine-arm added this pull request to the merge queue Aug 12, 2024

Merged via the queue into Mbed-TLS:development with commit 0858fdc Aug 12, 2024
4 checks passed

This was referenced Aug 13, 2024

Update framework to the head of the main branch #9474

Merged

3.6: Update framework to the head of the main branch #9475

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix build of v3.6 (issues #9186 and #9188) #9189

Fix build of v3.6 (issues #9186 and #9188) #9189

misch7 commented May 27, 2024 •

edited by minosgalanakis

Loading

gilles-peskine-arm left a comment

gilles-peskine-arm May 27, 2024

misch7 May 28, 2024

misch7 commented May 28, 2024

gilles-peskine-arm left a comment

gilles-peskine-arm left a comment

gilles-peskine-arm Jun 2, 2024

misch7 Jun 4, 2024 •

edited

Loading

gilles-peskine-arm Jun 2, 2024

misch7 Jun 4, 2024

gilles-peskine-arm Jun 4, 2024

misch7 Jun 4, 2024

misch7 Jun 4, 2024 •

edited

Loading

misch7 Jun 5, 2024

gilles-peskine-arm Jun 5, 2024

misch7 Jun 5, 2024 •

edited

Loading

gilles-peskine-arm Jun 2, 2024

misch7 Jun 4, 2024

gilles-peskine-arm Jun 4, 2024

misch7 Jun 4, 2024

gilles-peskine-arm commented Jun 5, 2024

gilles-peskine-arm commented Aug 9, 2024

minosgalanakis commented Aug 9, 2024 •

edited

Loading

misch7 commented Aug 9, 2024

misch7 commented Aug 9, 2024

misch7 commented Aug 9, 2024

gilles-peskine-arm left a comment

eleuzi01 left a comment

		The size of the array is thus greater than 256 bytes which is greater than any
		possible value of ecpoint_len (type uint8_t) and the check below can be skipped.*/

	for (size_t i = 0; i < thread_count; i++) {
	TEST_EQUAL(
	mbedtls_test_thread_create(&threads[i],
	thread_random_function, (void *) &ctx),
	0);
	}

	/* Wrapper for mbedtls_psa_platform_get_builtin_key */
	#if defined(MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS)
	psa_status_t mbedtls_test_wrap_mbedtls_psa_platform_get_builtin_key(
	mbedtls_svc_key_id_t arg0_key_id,
	psa_key_lifetime_t *arg1_lifetime,
	psa_drv_slot_number_t *arg2_slot_number)
	{
	psa_status_t status = (mbedtls_psa_platform_get_builtin_key)(arg0_key_id, arg1_lifetime, arg2_slot_number);
	return status;
	}
	#endif /* defined(MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS) */


	/** \def MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS
	*
	* Enable support for platform built-in keys. If you enable this feature,
	* you must implement the function mbedtls_psa_platform_get_builtin_key().
	* See the documentation of that function for more information.
	*
	* Built-in keys are typically derived from a hardware unique key or
	* stored in a secure element.
	*
	* Requires: MBEDTLS_PSA_CRYPTO_C.
	*
	* \warning This interface is experimental and may change or be removed
	* without notice.
	*/
	//#define MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS

		@@ -15,6 +15,8 @@

		#include <mbedtls/asn1.h>

		#include <test/asn1_helpers.h>

Fix build of v3.6 (issues #9186 and #9188) #9189

Fix build of v3.6 (issues #9186 and #9188) #9189

Conversation

misch7 commented May 27, 2024 • edited by minosgalanakis Loading

Description

PR checklist

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misch7 commented May 28, 2024

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misch7 Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misch7 Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misch7 Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Jun 5, 2024

gilles-peskine-arm commented Aug 9, 2024

minosgalanakis commented Aug 9, 2024 • edited Loading

misch7 commented Aug 9, 2024

misch7 commented Aug 9, 2024

misch7 commented Aug 9, 2024

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

eleuzi01 left a comment

Choose a reason for hiding this comment

misch7 commented May 27, 2024 •

edited by minosgalanakis

Loading

misch7 Jun 4, 2024 •

edited

Loading

misch7 Jun 4, 2024 •

edited

Loading

misch7 Jun 5, 2024 •

edited

Loading

minosgalanakis commented Aug 9, 2024 •

edited

Loading